Font family preview in the font family picker #67118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
matiasbenedetto
added
[Type] Enhancement
|
Size Change: +4 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d49960c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11915286928
|
tyxla
reviewed
Nov 20, 2024
There was a problem hiding this comment.
Delaying the review as feedback was provided in the related issue: #51151 (comment)
|
I updated the PR using the CustomSelectControl as suggested by @mirka in #51151 (comment). |
|
Thanks for moving this forward! I just tried to test this PR in WordPress playground and found that, while I could now see the Font Families, they didn't seem to properly update across the site, both when I made changes globally and to an individual block: Screen.Recording.2024-11-20.at.3.49.05.PM.movI'm not sure what's going on there. It might be Playground or something is off :) If it's playground, I'll report it to them. |
tyxla
requested changes
Nov 21, 2024
There was a problem hiding this comment.
Haven't tested, but I expect this will currently trigger a bunch of errors while rendering and while setting a new item. This is likely what @annezazu experienced.
Left a few suggestions that should resolve all issues.
I'd recommend keeping an eye on the console to ensure things work correctly and there are no errors.
| @@ -51,14 +53,14 @@ export default function FontFamilyControl( { | |||
| } | |||
|
|
|||
| return ( | |||
| <SelectControl | |||
| <CustomSelectControl | |||
| __next40pxDefaultSize={ __next40pxDefaultSize } | |||
| __nextHasNoMarginBottom={ __nextHasNoMarginBottom } | |||
There was a problem hiding this comment.
I think you need to remove this prop.
There was a problem hiding this comment.
Not sure what prop are you pointing to.
There was a problem hiding this comment.
If you mean __next40pxDefaultSize, I see it used in other parts of the codebase where CustomSelectControl is rendered, too:
There was a problem hiding this comment.
My comment is referring to the __nextHasNoMarginBottom prop, actually.
There was a problem hiding this comment.
ok, removed 1f94e6c
| __next40pxDefaultSize={ __next40pxDefaultSize } | ||
| __nextHasNoMarginBottom={ __nextHasNoMarginBottom } | ||
| label={ __( 'Font' ) } | ||
| options={ options } | ||
| value={ value } | ||
| onChange={ onChange } | ||
| labelPosition="top" |
There was a problem hiding this comment.
I think this prop also should be removed. Aren't you getting some errors in the console when passing this prop? I think you should, since it will end up being wrongly passed down to the underlying DOM element.
There was a problem hiding this comment.
It looks like we are passing the options as described in the component docs. See the options description and data example in storybook: https://wordpress.github.io/gutenberg/?path=/docs/components-customselectcontrol--docs
There was a problem hiding this comment.
I don't see console errors and it is used like that in other parts of the codebase. Example:
There was a problem hiding this comment.
Maybe you are referring something that is valid for the CustomSelectControlV2 ? I saw that control in the codebase, but it's not listed on storybook, and it doesn't seem to be in use in the Gutenberg codebase, so I'm not sure what it's their current state.
There was a problem hiding this comment.
My comment is referring to the labelPosition prop, not the options prop.
There was a problem hiding this comment.
removed here: 1f94e6c
|
Thanks for the reviews! With the latest change in Screencast.from.2024-11-21.08-41-31.mp4 |
@annezazu Thanks again for testing! I tried it in Playground, and it's working fine now: Screencast.from.2024-11-21.08-56-43.mp4 |
tyxla
reviewed
Nov 21, 2024
Thanks for the reviews. Those 2 props were removed in this commit: 1f94e6c |
mirka
approved these changes
Nov 21, 2024
|
@tyxla, your review is blocking the merge of this PR. Could you please take another look or lift the block? Thanks! |
tyxla
approved these changes
Nov 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Style font family option in the font family picker.
Why?
To help users to select the desired font.
Fixes: #51151
How?
Using the CustomSelectControl component and setting the right style.
Testing Instructions
Open a font family picker and verify that each font family renders the family's name using the family's typography.
Screenshot